Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add release build to github workflow #359

Merged
merged 2 commits into from
Feb 18, 2022

Conversation

jacobmerson
Copy link
Contributor

Adding release build to GitHub actions

see issue #358 . The current release build fails, but the GitHub actions tests are passing. To make sure we notice these issues cropping up I have (attempted) to add a release build to the GitHub actions workflow.

@jacobmerson
Copy link
Contributor Author

It turns out the release build wasn't the only issue causing my build to fail. That being said, we should probably be testing that both the debug and release builds work since bob.cmake inserts a bunch of flags depending on build type and most developers don't bother to check if code is compiling with multiple build types.

@jacobmerson
Copy link
Contributor Author

To replicate the errors I was seeing I updated ubuntu to use ubuntu-latest which will use more modern compilers which tends to catch more issues.

Ultimately, the -Werror flag should not be in the release build as discussed in #316

@cwsmith
Copy link
Contributor

cwsmith commented Feb 15, 2022

@jacobmerson This looks good. Thank you. Are you all set with this?

@jacobmerson
Copy link
Contributor Author

The pipeline works and I'm set with that. Do you need me to fix the build errors (existing issues in PUMI) before merge?

@cwsmith
Copy link
Contributor

cwsmith commented Feb 15, 2022

Either way (new PR or here) works for me.

@jacobmerson
Copy link
Contributor Author

As I track this down I noticed that the compiler is not getting selected correctly. gcc is used for the clang build as well.

@jacobmerson jacobmerson force-pushed the add-release-build branch 3 times, most recently from 8408519 to 2cd5506 Compare February 15, 2022 19:24
@jacobmerson jacobmerson reopened this Feb 17, 2022
@jacobmerson
Copy link
Contributor Author

@cwsmith we are nearing completion with this and you could probably merge as is (although I should cleanup the commit history). One interesting side effect is that the field_io test fails for the gcc release build. This may indicate that there is some undefined behavior lurking that creates a problem with the gcc optimizer. Do you need me to try to track this down before merge? Hopefully we can replicate with gcc-10 on the SCOREC systems.

If you are happy to merge as is, I will rebase the commits to make them nice.

Copy link
Contributor

@cwsmith cwsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobmerson LGTM. Thank you.

Should we set fail-fast to false so that the other matrix jobs aren't cancelled when gnu-release fails?
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-preventing-a-specific-failing-matrix-job-from-failing-a-workflow-run

Lets merge this and then resolve the field_io error in a separate PR.

The use of CMAKE_INSTALL_PREFIX causes issues when setting the prefix at
install time using cmake --install with --prefix
This commit cleans up the github workflow and adds a release build to
the build matrix. In addition it fixes an error where the correct
compiler was not being selected during build. Additionally, the
operating system has been updated to ubuntu-latest and gcc-10 is used.
@jacobmerson
Copy link
Contributor Author

@cwsmith I don't have merge permissions for core. Can you merge when you get a chance? I squashed the commits into something reasonable.

@cwsmith cwsmith merged commit 3250323 into SCOREC:develop Feb 18, 2022
@cwsmith cwsmith mentioned this pull request Mar 4, 2022
@cwsmith cwsmith added the v2.2.7 label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants